Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(lint): use golangci-lint to call revive and misspell checker. #18145

Merged
merged 5 commits into from
Jan 2, 2022
Merged

chore(lint): use golangci-lint to call revive and misspell checker. #18145

merged 5 commits into from
Jan 2, 2022

Conversation

appleboy
Copy link
Member

@appleboy appleboy commented Jan 1, 2022

replace revive and misspell with golangci-lint

Signed-off-by: Bo-Yi Wu appleboy.tw@gmail.com

@appleboy
chore(lint): remove revive and misspell checker.
4a091f2 
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Gusted
Gusted reviewed Jan 1, 2022
View reviewed changes
Makefile
@@ -280,29 +278,6 @@ errcheck:
@echo "Running errcheck..."
@errcheck $(GO_PACKAGES)

.PHONY: revive
revive:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see why we would remove revive, I'm not sure if it actually could be a drop-in as I know from golangci-lint that they don't always have 100% configuration as if you have with cmd command.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should integrate revive in golangci-lint?

Makefile
$(GO) install github.com/client9/misspell/cmd/misspell@v0.3.4; \
fi
@echo "Running misspell-check..."
@$(GO) run build/code-batch-process.go misspell -error -i unknwon '{file-list}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this shouldn't be touched, as it's using build/code-batch-process.go which seems to be special build made to batch this misspell action. Not sure why, maybe one of the other maintainers can shed some light on that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build/code-batch-process.go is only a wrapper for CLI.

In Windows, you can not pass more than 32K bytes via CLI arguments, so thecode-batch-process.go split the file names into batches. We can just ignore the wrapper during refacotoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ps: the build/code-batch-process.go came from #17684 to fix #14438 , so do not worry about it 😊

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 1, 2022
wxiaoguang
wxiaoguang reviewed Jan 1, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

For my understanding, this PR just removes revive and misspell.

The comment said replace revive and misspell with golangci-lint. How are they replaced?

@lunny
Copy link
Member

lunny commented Jan 1, 2022

golangci-lint now use revive internally #17609 but I think misspell is still necessary.

@appleboy
fix: revert two space
bc57210 
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy
chore(lint): enable revive lint.
c96f3ce 
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy
Copy link
Member Author

appleboy commented Jan 1, 2022

@wxiaoguang we can enable the revive lint in golangci-lint, see c96f3ce and misspell has been added in yaml file.

@lunny
Copy link
Member

lunny commented Jan 1, 2022

@wxiaoguang we can enable the revive lint in golangci-lint, see c96f3ce and misspell has been added in yaml file.

Which one is the misspell check?

@appleboy
Copy link
Member Author

appleboy commented Jan 1, 2022

@lunny See

gitea/.golangci.yml

Line 15 in 549fd03

- misspell

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 1, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@c99b8ef). Click here to learn what that means.
The diff coverage is 45.55%.

❗ Current head 549fd03 differs from pull request most recent head c96f3ce. Consider uploading reports for the commit c96f3ce to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18145   +/-   ##
=======================================
  Coverage        ?   45.01%           
=======================================
  Files           ?      825           
  Lines           ?    91557           
  Branches        ?        0           
=======================================
  Hits            ?    41215           
  Misses          ?    43756           
  Partials        ?     6586
Impacted Files Coverage Δ
cmd/web_letsencrypt.go 0.00% <0.00%> (ø)
models/lfs.go 23.96% <0.00%> (ø)
models/unittest/testdb.go 18.18% <0.00%> (ø)
routers/web/repo/lfs.go 0.00% <0.00%> (ø)
services/repository/adopt.go 26.69% <85.10%> (ø)
modules/indexer/code/bleve.go 66.52% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c99b8ef...c96f3ce. Read the comment docs.

Gusted
Gusted approved these changes Jan 2, 2022
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faster linting 🎉

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 2, 2022
@zeripath
Copy link
Contributor

zeripath commented Jan 2, 2022

make lgtm work

@zeripath zeripath changed the title chore(lint): remove revive and misspell checker. chore(lint): use golangci-lint to call revive and misspell checker. Jan 2, 2022
@zeripath zeripath merged commit 948949f into go-gitea:main Jan 2, 2022
@appleboy appleboy deleted the lint branch January 3, 2022 12:17
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 4, 2022
@zjjhot
Merge remote-tracking branch 'giteaoffical/main'
6cd8019 
* giteaoffical/main: (22 commits)
  Add MP4 as default allowed attachment type (go-gitea#18170)
  [skip ci] Updated translations via Crowdin
  Include folders into size cost (go-gitea#18158)
  Don't delete branch if other PRs with this branch are open (go-gitea#18164)
  Remove unused route "/tasks/trigger" (go-gitea#18160)
  Fix EasyMDE validation (go-gitea#18161)
  Fix bug (go-gitea#18168)
  tests: add coverage for models migration helpers  (go-gitea#18162)
  [skip ci] Updated translations via Crowdin
  Require codereview to have content (go-gitea#18156)
  chore(lint): use golangci-lint to call revive and misspell checker. (go-gitea#18145)
  Update owners for 2022 (go-gitea#18155)
  Refactor auth package (go-gitea#17962)
  Unify and simplify TrN for i18n (go-gitea#18141)
  Use correct user when determining max repo limits for error messages (go-gitea#18153)
  Add singuliere to MAINTAINERS (go-gitea#18148)
  [skip ci] Updated licenses and gitignores
  Add API to get issue/pull comments and events (timeline) (go-gitea#17403)
  Upgrade certmagic from v0.14.1 to v0.15.2 (go-gitea#18138)
  Upgrade certmagic from v0.14.1 to v0.15.2 (go-gitea#18138)
  ...
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 8, 2022 •
edited
Loading

The document should also be updated, eg: https://docs.gitea.io/en-us/hacking-on-gitea/#formatting-code-analysis-and-spell-check : make revive vet misspell-check

@appleboy appleboy mentioned this pull request Jan 8, 2022
Merged
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@appleboy
chore(lint): use golangci-lint to call revive and misspell checker. (g…
ebd52fd 
…o-gitea#18145)

replace revive and misspell with golangci-lint

Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
@wxiaoguang
Copy link
Contributor

--> Remove code-batch-process #21471

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@Gusted Gusted Gusted approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/code-linting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@appleboy @wxiaoguang @lunny @codecov-commenter @zeripath @Gusted @GiteaBot